Codecov Report

Merging #2272 (dac3058) into develop (db58a66) will increase coverage by 0.00%.
The diff coverage is 64.70%.

❗ Current head dac3058 differs from pull request most recent head 98dc106. Consider uploading reports for the commit 98dc106 to get more accurate results

@@           Coverage Diff            @@
##           develop    #2272   +/-   ##
========================================
  Coverage    59.75%   59.75%           
========================================
  Files           91       92    +1     
  Lines        16450    16455    +5     
  Branches      3796     3799    +3     
========================================
+ Hits          9829     9833    +4     
- Misses        6621     6622    +1     

Could I get a pass with "only" 73% coverage? I think I've covered the meaningful bits with tests.

The gate being 80% is actually considered low, James wanted the gate for the js-sdk to be 100% given it is a reusable SDK. The bits in topic.ts are fully covered, content-helpers is lacking one line and client.ts is at 59.3%.

https://sonarcloud.io/code?id=matrix-js-sdk&pullRequest=2272&selected=matrix-js-sdk%3Asrc%2Fcontent-helpers.ts&line=217
setRoomTopic looks entirely untested? https://sonarcloud.io/code?id=matrix-js-sdk&pullRequest=2272&selected=matrix-js-sdk%3Asrc%2Fclient.ts&line=3573

If you cover either of the above you should hit the gate. Are there any parts here which are infeasible to cover for some reason?

Could you also please pull latest develop, some of the checks aren't running seemingly

Are there any parts here which are infeasible to cover for some reason?

I suppose not infeasible, no. The main untested chunk setRoomTopic just seemed like it might not be worth covering because it mainly just calls sendStateEvent. I see the rationale for the coverage threshold though so let me see what I can do to meet it.

Could you also please pull latest develop, some of the checks aren't running seemingly

Hm, I had merged in develop just last night. 🤔 I'll merge it again when working on the tests. Hopefully that fixes it.

The main untested chunk setRoomTopic just seemed like it might not be worth covering because it mainly just calls sendStateEvent.

The added backwards compatibility overload is certainly worth testing, those are super fragile.

(Adding the label re-ran the missing CI job)

Hm, something's wrong. I've added tests for setRoomTopic and it's now fully covered. However, SonarCloud still rejects the PR with

74.6% Coverage on New Code (is less than 80%)

When I click through, it lists a lot of uncovered lines in "new" code for files that I haven't actually touched, like 81 lines in room.ts.

@t3chguy any idea what's wrong here?

I wonder if it just doesn't understand merge commits...

Not sure what happened here - #2366 was unaffected, showing 81.6% covg before & after the merge commit

Wonder if something has gone a bit strange on this branch given gh can't even rebase it

Skipping Sonar check due to weirdness, coverage is appropriate already

Thanks @t3chguy!